Anvil file support (blocks and biomes) #145
Conversation
|
Something worth mentioning: When loading chunks, especially with larger view distances, I notice (significant) stuttering while flying around in the This is room for improvement and should be looked into. This PR is a start, but at least we have more functionality than yesterday. |
Is the stuttering client side? I recall someone discovering their game was lagging because valence is not sending any light data. If the light data is filled with zeroes instead, the lag was eliminated. |
|
Game crashed while flying around with this. |
|
And there was also this. That row of stone blocks is not supposed to be there.
Quite possible that this is entirely my fault. |
There was a problem hiding this comment.
- Doc tests are failing. CI should have caught that.
- I feel that the program can be restructured to remove the Mutexes. It might make more sense to use channels. In other words, the API is structured where you can supply the chunks you want to be loaded and the program returns them through a channel as they're ready. This way, the example can be written where chunks are loaded into the valence server as they're ready without blocking the main server thread.
- Don't think there's much parallelism here? Looks like the
load_chunksfunction is processing chunks sequentially. - It is possible to have multiple file handles to the same file in read mode. Not sure if this should be taken advantage of.
- Using lava blocks to mark empty chunks is a bit confusing for the example (send a message to the client explaining this?).
- Might be good moving the example to the rest of examples and adding valence_anvil as a dev-dependency.
Not sure there's enough File IO happening to justify the use of tokio. Need to investigate how fastanvil is doing things.
valence_anvil/src/error.rs
Outdated
| @@ -0,0 +1,93 @@ | |||
| use std::error::Error as StdError; | |||
There was a problem hiding this comment.
In this module you can use #[derive(Error)] from the thiserror crate. That would save you some boilerplate.
anyhow::Error is boxed, meaning it is the size of only one pointer. Since errors are rare, this means anyhow::Result<T> is usually as small as possible which can have a meaningful performance impact in some cases. Perhaps we can do the same without being too dynamic/loose with errors. There are bigger fish to fry but something to consider.
There was a problem hiding this comment.
I have implemented the #[derive(Error)] in 8db39c7
This needs to be revisited when the parse_chunk_nbt function is being updated.
For future reference: #145 (comment)
valence_anvil/src/region.rs
Outdated
|
|
||
| //TODO: This function is very large and should be separated into dedicated | ||
| // functions at some point. | ||
| fn parse_chunk_nbt(nbt: &mut Compound, world: &AnvilWorld) -> Result<UnloadedChunk, Error> { |
There was a problem hiding this comment.
I think this function can be simplified by parsing the chunk NBT data (like what serde would typically do) separately from the block states, biomes, etc. rather than interleaving the work.
Also I realized the From traits we added to valence_nbt::Compound might be unidiomatic because From/Into conversions are not supposed to be lossy. Not too worried about that right now though.
There was a problem hiding this comment.
I have removed the From traits from valence_nbt for now just to be on the safe side. A good implementation might be to return an error type instead of None as shown in rust-lang/rfcs#2484.
|
Rather than just speculating about performance as I've done, I think it would be a good idea to set up a benchmark in |
…do not align. This fixes blocks randomly repeating at the start of chunk subsections
|
Here are some things I would like to see before we merge:
There are some API things we should figure out and some issues related to loading partially generated chunks, but let's not worry about that until after we merge. |
Reformat Biome parsing
There was a problem hiding this comment.
Now that I've had a chance to take a closer look at this, I have a lot to say. I've rewritten quite a few things.
Architectural Changes
Loading a Valence chunk is now done in a two-step process.
- Load the chunk NBT data from the filesystem.
- Convert the NBT data to a Valence chunk.
AnvilWorld still exists, but its only purpose is to complete step 1. Step 2 is done by a dedicated function named to_valence (in the future, we can add a from_valence to go the other way). This design has a number of advantages.
- All chunk data is accessible to users because the raw chunk NBT can be inspected after step 1.
valence_anvilno longer has a required dependency onvalence. If you only need step 1, there is no dependency. Step 2 is behind a feature flag.
Some details about to_valence:
- Rather than return an
UnloadedChunk,to_valenceaccepts a mutable reference to any type implementing theChunktrait. This is a lot more flexible. I've also made a few changes toChunkto make it more efficient. to_valenceaccepts a section offset. This lets us load the chunk at any height we need.to_valenceaccepts a function to map biome resource identifiers toBiomeId. This is convenient and means we don't need to store a map somewhere.
The vanilla biome generator has been removed for the time being. As far as Anvil and the protocol is concerned, vanilla biomes are not necessary. I still think it would be nice to have the vanilla biomes available so that we can implement the example correctly. This would go in the valence_protocol crate. We would probably also want to unify the Biome type in valence with the one in valence_protocol, but I didn't want to worry about all that in this PR.
The test for valence_anvil has been removed because I didn't want CI to have to download the world every time it runs. The benchmark does basically the same thing as the test, so we can still run that locally.
Speaking of benchmarks, the new code appears to be about 15% faster.
Code Feedback
-
The main issue I have is that your code seems to be designed around object-oriented classes rather than the procedures that are actually needed to solve the problem. This results in a lot of unnecessary complexity. Most of those classes don't add much value and I think you're better off without them. This includes types like
CompressionScheme,AnvilHeader,ChunkSeekLocation,ChunkTimestamp,RegionPos,WebAsset, etc. Your code is spread out across these classes' methods which makes things more difficult to follow in my opinion.In other words, if something is only used in one place, it's usually not worth making an abstraction for it. Don't add functionality until you actually need it.
-
To improve readability and decrease nesting, consider returning from functions as early as possible so that the "happy path" can continue with as little nesting as possible. The new
let elsefeature helps with this. You can see how I've used it into_valence. -
By convention, error messages start with a lowercase letter and do not end with punctuation.
-
Complicated trait bounds are better expressed using a
whereclause.
I'll go ahead and merge this now. If I missed something, feel free to open more PRs.
|
Thanks for the feedback! I see you made a lot of changes and it does look a lot better now. |
This is a starting point for the
valence_anvilcrate.It currently supports loading blocks and biomes.
Note on negative Y values:
Since
UnloadedChunkdoes not support negative Y values, all parsed Y values are raised to Y=0.Before merging, make sure the changes from 03e89ad are uploaded to crates.io and the version number is set in
Cargo.toml